test: enable marking of failing coverage tests #25671

mhdawson · 2019-01-23T22:15:32Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Enable marking of coverage tests so that we can
allow some tests to fail without blocking the generation
of coverage data. This will later allow us to
fail the coverage job if other kinds of errors occur and
to capture which tests we believe are not running properly
with coverage enabled.

This is step 1 of the following

allow us to run coverage and have failing tests not block generation of results
add support for a coverage level check (ex 90%) that will cause a failure @bcoe is working on that
add job to main PR regression test that will run coverage to make sure new failing tests are
not introduced
Update makefile so that we don't use '-' for the coverage target. At this point we will fail is
some other problem occurs but not if one of the known tests that affects coverage fails.

Note that this re-uses the '--type' option that we used for fips testing so that we can have entries
in the status files. The downside is that we would not be able to run coverage on FIPs as they
would conflict trying to use different types. I could add another option '--test-type' but I was
not sure if the added code/complexity is what people would want so I started with this approach.

Enable marking of coverage tests so that we can allow some tests to fail without blocking the generation of coverage data. This will later allow us to fail the coverage job if other kinds of errors occur and to capture which tests we believe are not running properly with coverage enabled.

Allow FAIL and CRASH to avoid yellow, otherwise if we added to regular CI run we'd have perma-yellow

Allow new tests to fail until we get a ci job added to the main regression job so that comits don't break coverage that have to be fixed later on.

nodejs-github-bot · 2019-01-23T22:15:34Z

@mhdawson build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2361/pipeline

mhdawson · 2019-01-23T22:17:29Z

FYI , job in the CI that I've been using for testing and that we'd want to add to the main regression PR.

https://ci.nodejs.org/job/node-test-commit-coverage/

One thing is which machines to run it on, currently coverage requires patch which does not seem to be installed on all of our machines. Maybe just run it on the machines we using for linting? @refack what do you think?

bcoe

mainly just had some questions, thank you for doing this.

bcoe · 2019-01-24T00:28:58Z

test/js-native-api/js-native-api.status

+[$system==aix]
+
+[$type==coverage]
+test_function/test: PASS,FAIL,CRASH


I like this approach to isolating flaky tests a lot 👍 first saw it in the blink codebase.

You might consider putting this in https://github.com/nodejs/node/blob/master/test/root.status
My intuition is that file fit better to represent this is a cross-cutting concern.

You learn something new every day. I was not aware of that option. I like having the list in one place so will move there.

bcoe · 2019-01-24T00:31:17Z

Makefile

@@ -226,7 +226,8 @@ coverage-test: coverage-build
 	$(RM) out/$(BUILDTYPE)/obj.target/node_lib/gen/*.gcda
 	$(RM) out/$(BUILDTYPE)/obj.target/node_lib/src/*.gcda
 	$(RM) out/$(BUILDTYPE)/obj.target/node_lib/src/tracing/*.gcda
-	-NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage $(MAKE) $(COVTESTS)
+	-NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage FLAKY_TESTS="dontcare" \


is this just to prevent FLAKY_TESTS from being overridden by an environment variable, so that js-native-api.status is used?

by default it is "run" which means it will fail if the tests crashes, this means if the test is marked flaky it will still not be marked as failed.

Can you double check that... Since you are not marking the tests FLAKY? Maybe we should keep this consistent with other CI target and just use dontcare as an override-able default?

Can we now remove the - prefix?

I was on the fence on overriding the dontcare. I think I'll remove for now.

In terms of removing - I don't want to do that until we have a job running (similar to the linter) in the main regression job. I want to avoid the case were a test goes in that breaks coverage and then somebody else (people keeing coverage going) are expected to fix it. As soon as we get the coverage check as part of the regression PR then we will remove it.

On the dontcare. To clarify I added that when I marked them as FLAKY but that did not actually work because the builds would then always be yellow which I don't think we wanted. Given that we'd probably we probably have to use FAIL or CRASH I removed the setting of dontcare.

bcoe · 2019-01-24T00:31:38Z

Makefile

@@ -277,7 +278,7 @@ coverage-run-js:
 	$(RM) -r out/$(BUILDTYPE)/.coverage
 	$(MAKE) coverage-build-js
 	-NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage CI_SKIP_TESTS=$(COV_SKIP_TESTS) \
-	  $(MAKE) jstest
+	        TEST_CI_ARGS="$(TEST_CI_ARGS) --type=coverage" $(MAKE) jstest


I think it would probably make sense to also specify:

FLAKY_TESTS="dontcare"

k will add.

based on @refack's comment I'll leave out setting dontcare completely for now.

refack

Left some questions

refack · 2019-01-24T16:50:49Z

Makefile

@@ -226,7 +226,8 @@ coverage-test: coverage-build
 	$(RM) out/$(BUILDTYPE)/obj.target/node_lib/gen/*.gcda
 	$(RM) out/$(BUILDTYPE)/obj.target/node_lib/src/*.gcda
 	$(RM) out/$(BUILDTYPE)/obj.target/node_lib/src/tracing/*.gcda
-	-NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage $(MAKE) $(COVTESTS)
+	-NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage FLAKY_TESTS="dontcare" \


Can you double check that... Since you are not marking the tests FLAKY? Maybe we should keep this consistent with other CI target and just use dontcare as an override-able default?

Can we now remove the - prefix?

refack · 2019-01-24T16:52:33Z

test/js-native-api/js-native-api.status

+[$system==aix]
+
+[$type==coverage]
+test_function/test: PASS,FAIL,CRASH


You might consider putting this in https://github.com/nodejs/node/blob/master/test/root.status
My intuition is that file fit better to represent this is a cross-cutting concern.

mhdawson · 2019-01-24T21:41:27Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/20311/

mhdawson · 2019-01-25T15:23:23Z

Opened issue for un-related failure on Windows: #25702

mhdawson · 2019-01-25T15:27:13Z

Created issue for unrelated issue on ARM - #25704

mhdawson · 2019-01-25T15:28:28Z

Resume build - https://ci.nodejs.org/job/node-test-pull-request/20321/

mhdawson · 2019-01-25T20:59:27Z

Resume again to try and get passed the arm issue: https://ci.nodejs.org/job/node-test-pull-request/20336/

mhdawson · 2019-01-28T22:34:28Z

One more time: https://ci.nodejs.org/job/node-test-pull-request/20385/

addaleax · 2019-01-28T23:16:39Z

I don’t think the ARM issue is resolvable through Resume CIs, sadly 😕

Rebuild CI: https://ci.nodejs.org/job/node-test-pull-request/20388/

danbev · 2019-01-29T07:24:43Z

Landed in c06653e.

Enable marking of coverage tests so that we can allow some tests to fail without blocking the generation of coverage data. This will later allow us to fail the coverage job if other kinds of errors occur and to capture which tests we believe are not running properly with coverage enabled. PR-URL: #25671 Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com>

mhdawson · 2019-01-30T13:58:41Z

@addaleax thanks. So that I know for the future is there a way to tell when an error won't work through resume CI or does that apply for ARM in general?

addaleax · 2019-01-30T13:59:57Z

@mhdawson I have no idea – I think the reason is that resume builds might not do an additional rebase against master, where the test had been fixed in the meantime…?

mhdawson added 3 commits January 23, 2019 16:06

squash: allow FAIL and CRASH to avoid yellow

58013ef

Allow FAIL and CRASH to avoid yellow, otherwise if we added to regular CI run we'd have perma-yellow

squash: allow new tests to pass until ci job added

9d0c898

Allow new tests to fail until we get a ci job added to the main regression job so that comits don't break coverage that have to be fixed later on.

nodejs-github-bot added build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jan 23, 2019

mhdawson requested review from bcoe and refack and removed request for bcoe January 23, 2019 22:16

bcoe approved these changes Jan 24, 2019

View reviewed changes

bcoe mentioned this pull request Jan 24, 2019

test: allow coverage threshold to be enforced #25675

Closed

2 tasks

BridgeAR approved these changes Jan 24, 2019

View reviewed changes

refack approved these changes Jan 24, 2019

View reviewed changes

refack added the coverage Issues and PRs related to native coverage support. label Jan 24, 2019

mhdawson added 3 commits January 24, 2019 16:04

squash: address comments

c27dd17

squash: address comments

a48bc9f

squash: address comments

f7b5b67

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 28, 2019

danbev closed this Jan 29, 2019

targos mentioned this pull request Jan 29, 2019

v11.9.0 proposal #25802

Merged

mhdawson deleted the coverage-tests branch September 30, 2019 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: enable marking of failing coverage tests #25671

test: enable marking of failing coverage tests #25671

mhdawson commented Jan 23, 2019

nodejs-github-bot commented Jan 23, 2019

mhdawson commented Jan 23, 2019

bcoe left a comment

bcoe Jan 24, 2019

refack Jan 24, 2019

mhdawson Jan 24, 2019

bcoe Jan 24, 2019

mhdawson Jan 24, 2019

refack Jan 24, 2019

mhdawson Jan 24, 2019

mhdawson Jan 24, 2019

bcoe Jan 24, 2019

mhdawson Jan 24, 2019

mhdawson Jan 24, 2019

refack left a comment •

edited

Loading

refack Jan 24, 2019

refack Jan 24, 2019

mhdawson commented Jan 24, 2019

mhdawson commented Jan 25, 2019

mhdawson commented Jan 25, 2019

mhdawson commented Jan 25, 2019

mhdawson commented Jan 25, 2019 •

edited

Loading

mhdawson commented Jan 28, 2019

addaleax commented Jan 28, 2019

danbev commented Jan 29, 2019

mhdawson commented Jan 30, 2019

addaleax commented Jan 30, 2019

test: enable marking of failing coverage tests #25671

test: enable marking of failing coverage tests #25671

Conversation

mhdawson commented Jan 23, 2019

Checklist

nodejs-github-bot commented Jan 23, 2019

mhdawson commented Jan 23, 2019

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Jan 24, 2019

mhdawson commented Jan 25, 2019

mhdawson commented Jan 25, 2019

mhdawson commented Jan 25, 2019

mhdawson commented Jan 25, 2019 • edited Loading

mhdawson commented Jan 28, 2019

addaleax commented Jan 28, 2019

danbev commented Jan 29, 2019

mhdawson commented Jan 30, 2019

addaleax commented Jan 30, 2019

refack left a comment •

edited

Loading

mhdawson commented Jan 25, 2019 •

edited

Loading